[experiment] Special handling of root node in AcyclicGraph algorithm#22014
Draft
ppedrot wants to merge 3 commits intorocq-prover:masterfrom
Draft
[experiment] Special handling of root node in AcyclicGraph algorithm#22014ppedrot wants to merge 3 commits intorocq-prover:masterfrom
ppedrot wants to merge 3 commits intorocq-prover:masterfrom
Conversation
Member
Author
|
@coqbot bench |
Member
Author
|
@SkySkimmer the bench died with a weird error in an unrelated script... Any idea what went wrong? |
Member
Author
|
Let's retry: @coqbot bench |
Contributor
|
That error happens when we tell it to bench an empty package. I did a manual run and opened #22015 |
Contributor
|
🏁 Bench results: INFO: failed to install coq-vst (dependency coq-compcert failed) 🐢 Top 25 slow downs┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 11.9 18.2 6.3512 53.51% 118 coq-fiat-parsers/src/Parsers/Refinement/SharpenedJSON.v.html │ │ 62.8 64.5 1.6061 2.56% 608 coq-bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 5.22 6.65 1.4352 27.52% 1346 coq-fiat-crypto-with-bedrock/src/Assembly/Symbolic.v.html │ │ 44.1 45.5 1.3076 2.96% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/bedrock2_fiat_crypto.v.html │ │ 42.6 43.9 1.2823 3.01% 578 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/MMIO.v.html │ │ 5.38 6.63 1.2436 23.10% 247 coq-fiat-crypto-with-bedrock/src/Util/FSets/FMapBool.v.html │ │ 7.03 8.10 1.0683 15.20% 552 coq-fiat-crypto-with-bedrock/src/Bedrock/P256/Jacobian.v.html │ │ 0.0103 1.08 1.0657 10316.61% 25 rocq-metarocq-erasure/erasure/theories/EEtaExpandedFix.v.html │ │ 38.6 39.6 1.0560 2.74% 1423 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 1.95 2.81 0.8586 44.06% 1003 coq-fiat-crypto-with-bedrock/src/Assembly/Symbolic.v.html │ │ 94.0 94.9 0.8446 0.90% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 1.60 2.41 0.8082 50.47% 309 coq-iris-examples/theories/logrel/F_mu_ref_conc/binary/examples/stack/refinement.v.html │ │ 4.67 5.48 0.8036 17.20% 66 coq-category-theory/Instance/Lambda/Eval.v.html │ │ 3.32 4.08 0.7575 22.83% 996 coq-fiat-crypto-with-bedrock/src/Assembly/Symbolic.v.html │ │ 237 237 0.6969 0.29% 141 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 0.377 1.03 0.6498 172.15% 565 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/redc.v.html │ │ 2.86 3.49 0.6280 21.98% 3602 rocq-metarocq-erasure/erasure/theories/Typed/OptimizeCorrectness.v.html │ │ 27.4 28.0 0.6273 2.29% 148 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/GarageDoorTop.v.html │ │ 29.2 29.8 0.6105 2.09% 145 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/GarageDoorTop.v.html │ │ 13.4 14.0 0.6029 4.49% 216 coq-fiat-crypto-with-bedrock/src/Fancy/Barrett256.v.html │ │ 1.72 2.31 0.5854 34.04% 38 coq-category-theory/Instance/Lambda/Exp.v.html │ │ 2.17 2.72 0.5517 25.43% 516 rocq-metarocq-common/common/theories/UniversesDec.v.html │ │ 0.274 0.819 0.5449 198.85% 389 coq-fiat-crypto-with-bedrock/src/Bedrock/P256/RecodeProofs.v.html │ │ 1.86 2.40 0.5367 28.83% 967 rocq-metarocq-safechecker/safechecker/theories/PCUICSafeRetyping.v.html │ │ 1.56 2.09 0.5292 33.98% 42 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 3.03 0.583 -2.4428 -80.74% 1479 rocq-metarocq-pcuic/pcuic/theories/PCUICSubstitution.v.html │ │ 202 200 -2.4232 -1.20% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 2.75 0.376 -2.3753 -86.32% 102 coq-fiat-parsers/src/Parsers/ParserImplementation.v.html │ │ 1.81 0.268 -1.5448 -85.20% 78 rocq-metarocq-pcuic/pcuic/theories/PCUICConversion.v.html │ │ 1.66 0.317 -1.3455 -80.96% 2493 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 1.57 0.230 -1.3429 -85.40% 2601 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 36.2 34.8 -1.3176 -3.64% 195 coq-fiat-crypto-with-bedrock/src/Bedrock/P256/Jacobian.v.html │ │ 1.28 0.197 -1.0820 -84.59% 395 rocq-metarocq-pcuic/pcuic/theories/PCUICElimination.v.html │ │ 1.25 0.202 -1.0525 -83.87% 278 rocq-metarocq-pcuic/pcuic/theories/Bidirectional/BDUnique.v.html │ │ 1.21 0.165 -1.0443 -86.34% 266 rocq-metarocq-pcuic/pcuic/theories/Bidirectional/BDUnique.v.html │ │ 1.18 0.177 -1.0016 -85.02% 298 rocq-metarocq-pcuic/pcuic/theories/Bidirectional/BDUnique.v.html │ │ 22.1 21.3 -0.8072 -3.65% 520 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/EdwardsXYZT.v.html │ │ 0.841 0.0378 -0.8031 -95.50% 245 rocq-metarocq-pcuic/pcuic/theories/PCUICSR.v.html │ │ 56.5 55.8 -0.7838 -1.39% 516 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/EdwardsXYZT.v.html │ │ 1.07 0.290 -0.7833 -73.00% 770 rocq-metarocq-pcuic/pcuic/theories/PCUICEquality.v.html │ │ 3.89 3.13 -0.7623 -19.58% 250 rocq-metarocq-erasure/erasure/theories/EWellformed.v.html │ │ 0.813 0.0567 -0.7564 -93.03% 383 rocq-metarocq-pcuic/pcuic/theories/PCUICClassification.v.html │ │ 55.8 55.1 -0.7468 -1.34% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 0.861 0.117 -0.7439 -86.41% 2583 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 0.836 0.0925 -0.7436 -88.94% 1801 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 0.870 0.129 -0.7405 -85.13% 407 rocq-metarocq-pcuic/pcuic/theories/PCUICConvCumInversion.v.html │ │ 0.863 0.127 -0.7360 -85.28% 1815 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 0.898 0.165 -0.7329 -81.64% 2470 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 0.847 0.124 -0.7240 -85.43% 2151 rocq-metarocq-safechecker/safechecker/theories/PCUICTypeChecker.v.html │ │ 0.758 0.0420 -0.7156 -94.45% 59 rocq-metarocq-pcuic/pcuic/theories/PCUICConvCumInversion.v.html │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
e487b6c to
1561417
Compare
We do not have to explicitly register it when building the empty graph.
The most expensive kind of level constraint in the current algorithm is of the form "enforce u ≤ Set". Indeed the forward pass has to crawl basically the whole graph starting from Set, as all levels are above Set. This is the worst case for the complexity of our variant of the Tarjan algorithm, which expects the graph to be sparse. Unfortunately, in practice such constraints seem to be commonplace in universe-polymorphic code. To work around this issue, in this commit we try to handle Set in a special way by not storing it in the graph as a normal node. Instead we assume "Set ≤ u" implicitly everywhere, and we only keep a set of constraints "Set < u". We still have to perform a kind of forward pass to merge a level with Set, but we do so hopefully efficiently by leveraging the topological order maintained by the algorithm.
1561417 to
bfc706e
Compare
Member
Author
|
The new version has the same fast-paths in the graph traversal for Set as for the normal algorithm. On the slowest line from fiat-parser it seems to be much faster. Let's see if this actually works on a new bench. @coqbot bench |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The most expensive kind of level constraint in the current algorithm is of the form "enforce u ≤ Set". Indeed the forward pass has to crawl basically the whole graph starting from Set, as all levels are above Set. This is the worst case for the complexity of our variant of of the Tarjan algorithm, which expects the graph to be sparse. Unfortunately, in practice such constraints seem to be commonplace in universe-polymorphic code.
To work around this issue, in this PR we try to handle Set in a special way by not storing it in the graph as a normal node. Instead we assume "Set ≤ u" implicitly everywhere, and we only keep a set of constraints "Set < u". We still have to perform a kind of forward pass to merge a level with Set, but we do so hopefully efficiently by leveraging the topological order maintained by the algorithm. We add in particular a map to quickly access nodes by their topological index.